DatePicker MergeStyles step 2 - Converts scss to js styles by bennettclark · Pull Request #5362 · microsoft/fluentui

bennettclark · 2018-06-27T22:58:12Z

Pull request checklist

fixes Datepicker: Convert to css-in-js #5365
Include a change request file using $ npm run change

Description of changes

Updates to DatePicker.base.tsx

adds import statements: classNamesFunction, customizable; IDatePickerStyleProps, IDatePickerStyles
adds getClassNames function to DatePicker.base.tsx
adds _classNames private function to DatePickerBase class
calls _classNames in public render() instead of styles

Removes DatePicker.scss file

Updates to DatePicker.styles.ts

adds import statements: Istyle (required by MergeStyles), normalize (to replace ms-normalize mixin from scss), getGlobalClassNames (required to apply classnames)
changes getStyles() to styles()
pulls { palette, fonts } from theme for semantic constants
replaces class name designations in the return statement with variables from classNames constant
converts all scss style rules from DatePicker.scss

Updates to DatePicker.tsx

changes getStyles() to styles()

Updates t DatePicker.types.ts

adds style type list from scss style rules to IDatePickerStyles for DatePicker.styles.ts to - the list must match between these two files (linter will complain if any of the list in types are not present in the styles file)

Updates snapshots

Microsoft Reviewers: Open in CodeFlow

…c-react into datepicker-mergestyles-step-2

JasonGore · 2018-06-27T23:14:40Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.styles.ts

+          }
+        }
+      },
+      className


Shouldn't className only be on root? I don't see it mixed in with other class names in old code

Yes, the standard className prop is a custom class name just for the root, not all elements.

JasonGore · 2018-06-27T23:30:20Z

...ages/office-ui-fabric-react/src/components/DatePicker/__snapshots__/DatePicker.test.tsx.snap

            aria-hidden={true}
            className=
-                ms-DatePicker-event--without-label
+


Hmm, I feel like this className should still be here but I don't see any obvious issues with the code

JasonGore · 2018-06-27T23:30:45Z

...ages/office-ui-fabric-react/src/components/__snapshots__/DatePicker.Bounded.Example.tsx.shot

-        className="ms-TextField"
+        className=
+            ms-TextField
+            ms-TextField


That's odd, I wonder why ms-TextField appears twice?

coming from TextField root, see my comment in DatePicker.styles.ts for the fix

JasonGore · 2018-06-27T23:31:07Z

.../office-ui-fabric-react/src/components/__snapshots__/DatePicker.WeekNumbers.Example.tsx.shot

+                  }
+                  &:not(.msDatePickerDisabled) {
+                    cursor: pointer;
+                    pointer-events: initial;


Do all of these styling changes match up with the new JS styling?

oengusmacinog-zz · 2018-06-28T03:02:05Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx

  prevYearAriaLabel: 'Go to previous year',
  nextYearAriaLabel: 'Go to next year'
 };
-


Nitpicky as all hell I realize, but can you put this blank line before the back? Separating declarations like the class and this interface really help with scan-ability

oengusmacinog-zz · 2018-06-28T03:05:46Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx

@@ -184,8 +196,7 @@ export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerS
              onClick: this._onIconClick,
              className: css(


This concatination logic should be in the styles.ts file. There css() function should no longer be used.

oengusmacinog-zz · 2018-06-28T03:07:14Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx

+  IDatePickerStyleProps,
+  IDatePickerStyles
+} from './DatePicker.types';
+import { BaseComponent, KeyCodes, createRef, css, classNamesFunction } from '../../Utilities';


css import should be removed - there's a completely redundant call on line 213 that shouldn't have been there in the first place (its 'concatenating' just one class with nothing else

oengusmacinog-zz · 2018-06-28T03:36:27Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.styles.ts

+          }
+        }
+      },
+      className


Yes, the standard className prop is a custom class name just for the root, not all elements.

oengusmacinog-zz · 2018-06-28T03:46:35Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.styles.ts

-  const { className } = props;
+const GlobalClassNames = {
+  root: 'ms-DatePicker',
+  textField: 'ms-TextField',


duplicate - 'ms-TextField' comes from the TextField component root, so when they combine you render two 'ms-TextField' s - the scss implementation didn't have a class for this element just styles.

Yeah, this className should be removed

oengusmacinog-zz · 2018-06-28T03:47:22Z

...ages/office-ui-fabric-react/src/components/DatePicker/__snapshots__/DatePicker.test.tsx.snap

 exports[`DatePicker renders default DatePicker correctly 1`] = `
 <div
-  className="ms-DatePicker"
+  className={undefined}


this should not be undefined

Per our Teams chat, DatePicker.test.tsx snapshot test should be modified to use DatePicker instead of DatePickerBase.

oengusmacinog-zz · 2018-06-28T03:48:11Z

...ages/office-ui-fabric-react/src/components/__snapshots__/DatePicker.Bounded.Example.tsx.shot

-        className="ms-TextField"
+        className=
+            ms-TextField
+            ms-TextField


coming from TextField root, see my comment in DatePicker.styles.ts for the fix

JasonGore · 2018-06-28T16:02:35Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx

 import { TextField, ITextField } from '../../TextField';
-import { BaseComponent, KeyCodes, css, createRef } from '../../Utilities';
 import { compareDates, compareDatePart } from '../../utilities/dateMath/DateMath';
 import * as stylesImport from './DatePicker.scss';


I'm surprised stylesImport isn't causing an error since the file appears to have been removed

JasonGore · 2018-06-28T16:02:46Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx

 import { compareDates, compareDatePart } from '../../utilities/dateMath/DateMath';
 import * as stylesImport from './DatePicker.scss';
 import { FocusTrapZone } from '../../FocusTrapZone';
 const styles: any = stylesImport;


styles should no longer be used anywhere and removed

…c-react into datepicker-mergestyles-step-2

…removes textfield class injection

…c-react into datepicker-mergestyles-step-2

JasonGore · 2018-06-28T23:49:03Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.styles.ts

  return {
-    root: [
-      'ms-DatePicker',
+    root: [classNames.root, isDatePickerShown && 'is-open', normalize, {}, className],


Shouldn't need the empty object here

…c-react into datepicker-mergestyles-step-2

oengusmacinog-zz

Looks great

cliffkoh

Could you please rename DatePicker.tsx to DatePicker.ts (it has no JSX in it)?

cliffkoh · 2018-06-29T19:06:37Z

packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.styles.ts

-export const getStyles = (props: IDatePickerStyleProps): IDatePickerStyles => {
-  const { className } = props;
+const GlobalClassNames = {
+  root: 'ms-DatePicker',


* master: (151 commits) Css updates for card components (microsoft#5393) DatePicker MergeStyles step 2 - Converts scss to js styles (microsoft#5362) Update stale with 60 day comment and updated ignored label names. (microsoft#5388) replaced await for ie compat syntax - but still needs promise (microsoft#5387) Slider - mergestyle conversion (microsoft#5379) Applying package updates. Enabling publishing for charting and gridlayout. (microsoft#5378) TilesList: fadeOut overlay. (microsoft#5381) Pivot: JS Styling (microsoft#5324) ShimmeredDetailsList: wrapper for DetailsList with Shimmer. (microsoft#5374) dashboard-grid-layout improved example (microsoft#5373) Applying package updates. Addressing Issue microsoft#5165 - Using Customizer with Nav Component (microsoft#5361) Applying package updates. Address issue microsoft#5353 (microsoft#5359) Change log to trigger release (microsoft#5358) Fluent updates (microsoft#5357) TextField render value undefined as empty string (microsoft#5349) Applying package updates. DetailsColumn: css class changes to show gripper when hover on draggable columns (microsoft#5309) ...

bennettclark added 6 commits June 27, 2018 11:14

datepicker styles conversion progress

3813de8

datepicker styles updates

4ef49bf

^finalizes datepicker scss conversion to mergestyles

ff1cdd1

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

7867df2

…c-react into datepicker-mergestyles-step-2

checks over DatePicker styles and updates snapshots

4fb2b33

changelog

efb94d4

bennettclark requested a review from lorejoh12 as a code owner June 27, 2018 22:58

bennettclark requested review from JasonGore, mikewheaton, oengusmacinog-zz and phkuo June 27, 2018 22:58

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

ed7ff40

…c-react into datepicker-mergestyles-step-2

JasonGore reviewed Jun 27, 2018

View reviewed changes

oengusmacinog-zz suggested changes Jun 28, 2018

View reviewed changes

JasonGore reviewed Jun 28, 2018

View reviewed changes

bennettclark added 4 commits June 28, 2018 09:30

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

e923dbd

…c-react into datepicker-mergestyles-step-2

removes all use of css, creates style concatenation for icon styles, …

c3fe7c4

…removes textfield class injection

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

f3209a5

…c-react into datepicker-mergestyles-step-2

^updates snapshots

79a4c25

JasonGore reviewed Jun 28, 2018

View reviewed changes

mikewheaton approved these changes Jun 29, 2018

View reviewed changes

bennettclark added 3 commits June 29, 2018 09:47

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

1fa4871

…c-react into datepicker-mergestyles-step-2

fixes test to use datepicker and datepickerbase appropriately

fec4744

removes unused empty object from root styles

f22cb19

oengusmacinog-zz approved these changes Jun 29, 2018

View reviewed changes

cliffkoh approved these changes Jun 29, 2018

View reviewed changes

lorejoh12 approved these changes Jun 29, 2018

View reviewed changes

lorejoh12 merged commit 78c5cab into microsoft:master Jun 29, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

		@@ -184,8 +196,7 @@ export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerS
		onClick: this._onIconClick,
		className: css(

Conversation

bennettclark commented Jun 27, 2018 • edited by micahgodbolt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oengusmacinog-zz left a comment

Choose a reason for hiding this comment

Uh oh!

cliffkoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bennettclark commented Jun 27, 2018 •

edited by micahgodbolt

Loading

JasonGore Jun 29, 2018 •

edited

Loading

JasonGore Jun 28, 2018 •

edited

Loading

JasonGore Jun 28, 2018 •

edited

Loading